fix(policy): resolve this.relation.field against @@allow model, unnest array columns in subqueries#2734
Open
hechang27-sprt wants to merge 2 commits into
Conversation
…t array columns in subqueries Fix zenstackhq#1: In _member() isThis branch, pass modelOrType: context.thisType to transformRelationAccess() so that this.relation.field correctly resolves relation fields against the @@Allow model (thisType) rather than the collection predicate's model (modelOrType). Fix zenstackhq#2: In _member() when the innermost field of a subquery selection is an array type AND the provider is PostgreSQL, wrap the column with unnest() so that ANY(subquery) receives scalar rows instead of array rows. Other providers (SQLite, MySQL) leave the column as-is since they store arrays differently (JSON) and would need different handling.
Contributor
📝 WalkthroughWalkthroughThe policy expression transformer now resolves chained ChangesPolicy relation and array IN fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
hechang27-sprt
force-pushed
the
fix/this-authscope-relation-resolution
branch
from
1965841 to
cfbc2ce
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/plugins/policy/src/expression-transformer.ts`:
- Around line 1073-1082: The buildArrayInExists path is reconstructing a new
select from only the first table and blindly reading subquery.where, which
breaks many-to-many relation queries that carry correlation in joins and may
have no where clause. Update buildArrayInExists to work from the existing
SelectQueryNode shape, preserving any joins and other query parts instead of
using froms[0] alone, and append the containment predicate to the incoming
subquery rather than dereferencing subquery.where! or rebuilding the query from
scratch.
- Around line 712-715: The first `this` relation hop in
`expression-transformer.ts` is still carrying `restContext.alias`, which can
resolve to the wrong table alias in nested policy predicates. Update the
`transformRelationAccess` call in this branch so that when `modelOrType` is
switched back to `context.thisType`, the alias also uses `context.thisAlias` for
consistency with the one-segment `this.foo` handling, keeping the outer
`@@allow` model reference stable.
- Around line 257-262: The array-subquery branch in expression-transformer is
returning early before the enum scalar cast logic runs, so enum comparisons skip
the same normalization used by the non-subquery any(...) path. Update the
handling in ExpressionTransformer around buildArrayInExists so the right-hand
expression is still passed through the existing enum scalar casting logic before
routing to buildArrayInExists, keeping the cast behavior consistent for
array-column subqueries and regular any(...) comparisons.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f6aeb4a-8549-4cdc-82d9-5aa6b985dfd8
📒 Files selected for processing (2)
packages/plugins/policy/src/expression-transformer.tstests/e2e/orm/policy/auth-access.test.ts
hechang27-sprt
force-pushed
the
fix/this-authscope-relation-resolution
branch
2 times, most recently
from
da34156 to
34d86b9
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/plugins/policy/src/expression-transformer.ts (1)
1083-1128: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve joins when rebuilding the non-PostgreSQL
EXISTS.Line 1127 creates a fresh
selectFrom(tableName), so anysubquery.joinsfrom relation access—especially many-to-many joins fromtransformManyToManyRelationAccess—are dropped. That can make SQLite/MySQL array membership checks uncorrelated or incorrect even thoughsubquery.whereis merged.Verification script
#!/bin/bash # Inspect relation subquery shapes and generated array-in helper call sites. ast-grep outline packages/plugins/policy/src/expression-transformer.ts --match ExpressionTransformer --view expanded rg -n -C4 'buildArrayInExists|transformManyToManyRelationAccess|subquery\.joins|selectFrom\(tableName\)' packages/plugins/policy/src/expression-transformer.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/plugins/policy/src/expression-transformer.ts` around lines 1083 - 1128, The non-PostgreSQL EXISTS rebuild in expression-transformer drops relation joins by creating a new selectFrom(tableName), which breaks many-to-many and other correlated array checks. Update the logic around buildArrayInExists so it reuses the existing subquery shape, including subquery.joins, when constructing the final EXISTS for SQLite/MySQL, while still merging subquery.where with the array membership predicate. Keep the original table/alias handling from transformManyToManyRelationAccess and preserve all join conditions instead of starting a fresh root query.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/plugins/policy/src/expression-transformer.ts`:
- Around line 1083-1128: The non-PostgreSQL EXISTS rebuild in
expression-transformer drops relation joins by creating a new
selectFrom(tableName), which breaks many-to-many and other correlated array
checks. Update the logic around buildArrayInExists so it reuses the existing
subquery shape, including subquery.joins, when constructing the final EXISTS for
SQLite/MySQL, while still merging subquery.where with the array membership
predicate. Keep the original table/alias handling from
transformManyToManyRelationAccess and preserve all join conditions instead of
starting a fresh root query.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e26234ba-921d-477d-8c75-4d3c1efeac5c
📒 Files selected for processing (2)
packages/plugins/policy/src/expression-transformer.tstests/e2e/orm/policy/auth-access.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/orm/policy/auth-access.test.ts
…-db array-in EXISTS Fixes zenstackhq#2733. Fix 1: `_member` isThis branch now passes `modelOrType: context.thisType` to `transformRelationAccess`, so `this.relation.field` resolves against the @@Allow model instead of the collection predicate model. Fix 2: `this.scalarField in this.relation.arrayField` now generates a cross-database EXISTS subquery instead of `= ANY(subquery)`. Each provider uses its native array operator: - PostgreSQL: EXISTS (SELECT 1 FROM t WHERE join AND scalar = ANY(t.col)) - SQLite: EXISTS (... AND EXISTS (SELECT 1 FROM json_each(t.col) WHERE value = scalar)) - MySQL: EXISTS (... AND EXISTS (SELECT 1 FROM JSON_TABLE(t.col, ...) AS jt WHERE jt.value = scalar)) Also fixes `getFieldDefFromFieldRef` to resolve multi-segment `this.relation.field` chains, needed to detect array fields in the `in`-operator RHS. Tests: two new PG tests in auth-access.test.ts, verified to fail without the fix and pass with it.
hechang27-sprt
force-pushed
the
fix/this-authscope-relation-resolution
branch
from
34d86b9 to
9e78de7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2733.
Problem
Two bugs in the policy plugin expression transformer:
this.relation.fieldresolves against the wrong model insideauth().collection?[...]predicates. The expression transformer usescontext.thisTypeto resolve the field definition but passesrestContext(withmodelOrTypestill pointing to the collection model) totransformRelationAccess. This causesField "X" not found in model "<collection>"crashes.x in this.relation.arrayFieldgenerates broken SQL on PostgreSQL. The expression compiles to= ANY((SELECT arrayCol FROM ...))where the subquery returns array-typed rows (integer[]). PostgreSQLANY(subquery)requires scalar rows, producingoperator does not exist: integer = integer[].Fix
Fix 1: Resolve
this.relation.fieldagainst the@@allowmodelPass
modelOrType: context.thisTypeandalias: context.thisAliastotransformRelationAccesswhen the receiver isthis, so relation fields and table aliases resolve against the model bearing the policy. Also fixedgetFieldDefFromFieldRefto resolve multi-segmentthis.relation.fieldchains (needed for the array-field detection in Fix 2).Fix 2: Cross-database
scalar IN relation.arrayFieldSingle entry point
buildArrayInExistsin_binary, with provider-specific handling:unnest()in subquery column += ANY(subquery)scalar = ANY(SELECT unnest("col") FROM "t" WHERE <join>)json_eachEXISTS (SELECT 1 FROM "t" WHERE <join> AND EXISTS (SELECT 1 FROM json_each("t"."col") WHERE "value" = scalar))JSON_TABLEEXISTS (SELECT 1 FROM "t" WHERE <join> AND EXISTS (SELECT 1 FROM JSON_TABLE(...) AS jt WHERE jt."value" = scalar))The SQLite/MySQL
EXISTSrebuild preserves the original subqueryFROMclause, so many-to-many joins are kept intact.Verification
Two new PG-specific tests added to
auth-access.test.ts:this.author.levelinauth().permissions?[p, ...]this.id in this.group.visibleDocIdsAll existing policy tests continue to pass on PostgreSQL, SQLite, and MySQL (no regressions).